Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Indexed Database API (http://www.w3.org/TR/IndexedDB). #319

Merged
merged 8 commits into from
Jan 25, 2013
Merged

Added Indexed Database API (http://www.w3.org/TR/IndexedDB). #319

merged 8 commits into from
Jan 25, 2013

Conversation

aschoenebeck
Copy link
Contributor

Only asynchronous database API implemented. But this should be the default anyway. Issue was #316.

@aschoenebeck aschoenebeck mentioned this pull request Jan 16, 2013
@nikhilk
Copy link
Owner

nikhilk commented Jan 17, 2013

Couple of quick comments on an initial quick pass over the diff, but first thanks ... it will be great to have coverage for this part of the HTML5 scenarios and APIs.

  • The I in front of every class name is a bit confusing for c# developers, who are probably more used to seeing that used for interfaces. Could we just use IndexedDB as the the prefix instead for these types?
  • The IDBEvent class should probably be IndexedDBEventArgs and derive from EventArgs. And to go hand-in-hand, the ???Delegate types should probably be ???EventHandler types. Its uncommon to see Delegate in the actual names of delegate types.
  • Does IndexedDB support addEventListener/removeEventListener style of event registration rather than setting properties like OnAbort? Haven't tried or looked into the spec. However, if it did, that would allow events to be mapped as actual events in c# and have the compiler generate script to add/removeEventListener pattern.
  • Alternatively, if these are conceptually better thought of as callbacks, then the delegate types would be better named ???Callback and the IDBEvent types as some sort of notification/message types.

@aschoenebeck
Copy link
Contributor Author

Hi nikhil,

  • The IDB prefix is for IndexedDB. All classes and are officially named like that in the spec.
  • I will change IDBEvent to IDBEventArgs and derive from EventArgs. I will see about EventHandler types for callbacks.
  • IndexedDB object do support the addEventListener/removeEventListener style e.g. interface IDBRequest : EventTarget { ... }. Can you show me an example how to implement this pattern on IDB classes?
  • Naming these ...Callback is fine as an alternative.

@nikhilk
Copy link
Owner

nikhilk commented Jan 17, 2013

So most of the HTML specs are actually defining interfaces, and seems reasonable they have an "I" prefix. I think that fact + combined with the desire to make the model not completely out of place in the c# world, I think we should honor those naming patterns.

See this for an example of using [ScriptEvent] to customize events. https://github.com/nikhilk/scriptsharp/blob/cc/src/Libraries/Node/Node.Core/IO/WritableStream.cs

So we'd have IndexedDBEventArgs, and types such as IndexedDBEventHandler where TEventArgs is a type derived from IndexedDBEventArgs.

@aschoenebeck
Copy link
Contributor Author

The javascript spec IDL "interfaces" are class interfaces and not interfaces in the c# sense. The "I" in IDB comes from IndexedDB, which is unfortunate, but well...

I've used the [ScriptEvent] for all callbacks and renamed the delegates to ...callback.

I'll keep IDBEvent and not use EventArgs and EventHandler<>. IndexedDB callbacks are not providing the sender object in callbacks so EventHandler<> is wrong.

@aschoenebeck aschoenebeck reopened this Jan 17, 2013
The IndexedDB API specifies two methods for event registration on its
interfaces. Direct callback funtions and the EventTarget scheme.

To make the interface as standards conformant as possible, I derive
classes providing callbacks from IDBEventTarget base class (proviedes
add-/removeEventListener) and provide fields for direct callback
functions.
@aschoenebeck
Copy link
Contributor Author

The IndexedDB API specifies two methods for event registration on its interfaces. Direct callback funtions and the EventTarget scheme.

To make the interface as standards conformant as possible, I now derive classes providing callbacks from IDBEventTarget base class (provides add-/removeEventListener) and provide fields for direct callback functions as before.

Now to register the handler you can do it both ways, just like the standard defines:

        IDBOpenDBRequest openrequest = Window.IndexedDB.Open(storagekey);
        openrequest.OnSuccess = delegate(IDBEvent<IDBRequest> e) {
            // Direct callback
        };
        openrequest.AddEventListener("success", delegate(IDBEvent<IDBRequest> e) {
            // EventTarget callback
        });

@nikhilk
Copy link
Owner

nikhilk commented Jan 18, 2013

So, there is no reason really to propagate what you called "unfortunate". There are other precedents for writing imported APIs such that they work better or more naturally in the c# context. Seeing the code above makes me even more convinced the "I" prefix is misleading.

If you're really averse to putting "Indexed" as the prefix, I think we should move all of this to an "IndexedDB" subnamespace (Html.Data.IndexedDB), and have all the classes simply be DBOpenRequest, DBEvent etc. I think this will be generally good anyway, as we don't want a mix of WebSQL and IndexedDB APIs in the same namespace. I'll move the SQL ones into a subnamespace as well.

Some more comments.

Why do we need both OnSuccess and AddEventListener("success")? At the c# level, we should simply surface the event as an actual event member (assumption is there is no behavioral difference as far as the event handler is concerned), because its confusing which one to pick... the natural event syntax or the property assignment.

Thanks...

@aschoenebeck
Copy link
Contributor Author

Now I have renamed the classes to DBRequest etc. and put them in their own namespace System.Html.Data.IndexedDB. Nice and clean.

Regarding the callbacks, I have at least one good use for the callback fields. There is a javascript IndexedDB shim I use for older browsers and that only supports simple callback fields.

@aschoenebeck
Copy link
Contributor Author

So is this ready to merge or are there more things to discuss/change?

@nikhilk
Copy link
Owner

nikhilk commented Jan 22, 2013

I think this is good. There are some things around coding style guidelines, but I'll take care of those so as to not prolong this any further. Hope to merge the pull request in next day or two.

nikhilk added a commit that referenced this pull request Jan 25, 2013
@nikhilk nikhilk merged commit b517c1b into nikhilk:cc Jan 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants